Don't cancel fetchQuery when unmounting useQuery #9799
Conversation
🦋 Changeset detectedLatest commit: ebe1535 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe PR modifies query cancellation semantics in TanStack Query. When Changes
Sequence DiagramsequenceDiagram
participant Client as queryClient.fetchQuery
participant Observer as QueryObserver
participant Query as Query
participant Cache as Cache
Client->>Client: Check if data is stale
alt Data is stale
Client->>Observer: Create and attach observer
Observer->>Query: Subscribe to query
Client->>Query: Execute fetch
Query->>Cache: Update data
Client->>Observer: Remove observer (finally)
Note over Client,Cache: Query remains in cache<br/>not cancelled by other<br/>observer unsubscribes
else Data is fresh
Client->>Cache: Return cached data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce logic modifications to fetchQuery's control flow with observer lifecycle management, span multiple test files with varying adjustments, and require understanding interactions between QueryObserver and cancellation semantics. Changes are moderately heterogeneous across implementation and tests. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.changeset/petite-towns-rule.md (1)
5-5: Tighten wording and format the API name.Suggest: “When running
queryClient.fetchQuery, the query will no longer be cancelled if other observers unsubscribe.” This adds code formatting, active voice, and the more common “unsubscribe.”packages/query-core/src/queryClient.ts (1)
366-379: Using a temporary QueryObserver to prevent cancellation is sound; add small polish.
- Behavior change is correct: keeping one observer prevents “last-unsubscribe” cancellation while
fetchQueryis in flight. LGTM.- Minor: adding the observer via
query.addObserver(observer)(without listeners) can still emit cache notifications fromobserver.updateResult(); acceptable, but worth noting for large subscriber counts.- Optional polish: keep style consistent and use a clearer try/finally.
- const isDataStale = query.isStaleByTime( - resolveStaleTime(defaultedOptions.staleTime, query), - ); + const isDataStale = query.isStaleByTime( + resolveStaleTime(defaultedOptions.staleTime, query), + ) @@ - const observer = new QueryObserver(this,defaultedOptions) - query.addObserver(observer); - - return query.fetch(defaultedOptions).finally(() => query.removeObserver(observer)) + const observer = new QueryObserver(this, defaultedOptions) + query.addObserver(observer) + try { + return await query.fetch(defaultedOptions) + } finally { + query.removeObserver(observer) + }Please double‑check that this path preserves existing pause/resume semantics (offline/focus) by running the affected tests for paused retries and onFocus/onOnline refetch.
packages/query-core/src/__tests__/query.test.tsx (1)
294-305: Timer alignment nit.The added 10ms advance before unsubscribe makes the race explicit. Consider adding a brief inline comment “start fetch → unsubscribe after 10ms” for clarity.
packages/react-query/src/__tests__/useQuery.test.tsx (1)
6780-6845: Solid coverage for “don’t cancel fetchQuery on unmount”; fix comment.The test correctly asserts that the AbortSignal remains un‑aborted and
fetchQueryresolves. Minor nit: comment says “after 2 seconds” but timers use ~10–11ms; update the comment to avoid confusion.- // This unmounts useQuery after 2 seconds, fetchQuery should continue running + // This unmounts useQuery after ~10ms; fetchQuery should continue running
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/petite-towns-rule.md(1 hunks)packages/query-core/src/__tests__/query.test.tsx(3 hunks)packages/query-core/src/queryClient.ts(2 hunks)packages/react-query/src/__tests__/useQuery.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/react-query/src/__tests__/useQuery.test.tsx (3)
packages/react-query/src/queryOptions.ts (1)
queryOptions(85-87)packages/query-core/src/utils.ts (1)
sleep(363-367)packages/react-query/src/__tests__/utils.tsx (1)
renderWithClient(9-23)
packages/query-core/src/__tests__/query.test.tsx (1)
packages/query-core/src/utils.ts (1)
sleep(363-367)
packages/query-core/src/queryClient.ts (3)
packages/query-core/src/queryObserver.ts (2)
query(704-721)QueryObserver(41-747)packages/query-core/src/utils.ts (1)
resolveStaleTime(101-113)packages/query-core/src/queriesObserver.ts (1)
observer(263-269)
🔇 Additional comments (3)
packages/query-core/src/queryClient.ts (1)
16-16: Import looks correct and scoped to core.
No concerns.packages/query-core/src/__tests__/query.test.tsx (1)
216-238: Good AbortSignal consumption check.Adding
signal.throwIfAborted()and asserting resolved data validates the new non‑cancellation behavior offetchQuery.packages/react-query/src/__tests__/useQuery.test.tsx (1)
16-16: queryOptions export confirmed
packages/react-query/src/index.tsline 20 re-exportsqueryOptions— no further action needed.
|
The behavior is if there are no observers, the query is inactive and scheduled for garbage collection, but I think it is indeed unintuvitive that an imperative fetch gets canceled. |
| const observer = new QueryObserver(this,defaultedOptions) | ||
| query.addObserver(observer); | ||
|
|
||
| return query.fetch(defaultedOptions).finally(() => query.removeObserver(observer)) |
There was a problem hiding this comment.
I think we'd want a way to differentiate imperative fetches from observer fetches, rather then this temp observer hack.
|
sorry I don’t think this is the way to go. |
3 participants
🎯 Changes
This is an attempt to solve issue #9798.
It adds an query observer when running
fetchQuery, to make sure the query is not cancelled if all other observers are unsubscribed.I am unfamiliar with this codebase, and at least one test is still failing. I am unsure if this is the right direction to fix the issue. If anyone wants to take over this PR, feel free to do so.
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
fetchQuerywould cancel in-progress queries when other observers unsubscribed. Queries now continue to completion as expected.